Total-order planner - #645

This looks really nice -- great work!

There are too many conflicts for me to resolve -- can you have a look?

One of the main issues is that the PlanningKB items are immutable (and therefore hashable and searchable) and the FOLKB items from logic.py aren't. I'll need some time to see if I can make these two object models work together, and probably a decision from you as to which way to go with the implementation.

Hey @bcorfman, I know that Expr objects have a __hash__ method. I think that FOLKB should only be using these; do you know where they are not used?

Hi Dr. Norvig - I use the existing A* algorithm to solve the example planning problems. Recall that the set of explored items is maintained during the search to keep track of repeated states (line 270 in the current search.py). Any items in this set must be hashable and unique, or they will fail when they are added to the set. I use my PlanningKB as the state variable in the search Node, and it's hashable because the PlanningKB only uses frozenset variables instead of regular mutable sets. The FolKB database doesn't work as the Node.state because it's designed (because of its tell and ask methods) to be mutable -- that means its clauses variable is a mutable list, and its state can't be hashed.

I got it. What about if you use tuple(kb.clauses) for the state? You could then put the clauses back into a KB when you need to.

Yep, a tuple should work for the FolKB clauses. (I remember using tuples with the Node setup for one of the Berkeley Pac-Man projects.) I guess my question is whether you want to a) just make the FolKB usable for the search algorithms, 2) combine the existing logic.py and my new planning.py code together, or 3) just pass the FolKB clause list into the PlanningKB init and then use the PlanningKB to conduct the search. I think number 3 is the easiest -- although there'd be two KBs for different purposes, my PlanningKB and Action classes use frozenset methods to quickly perform goal and precondition checks. I can make changes based on what you think is the best long-term approach.

I also added a couple of classmethods to my PlanningProblem and PlanningAction classes. These alternative constructors (PlanningProblem.from_PDDL_object and PlanningAction.from_action) take existing PDDL and Action objects as parameters in order to convert the KB and actions to be searchable.

If you decide to use my PDDL parser too, I might suggest renaming the PDDL class to something more appropriate -- there's starting to be a lot of things named "PDDL" in the code, and it could create confusion.

I think this looks pretty good, but there's a lot here stretching over a long time. Can you, @bcorfman , check in with @ad71 and perhaps @samagra14 over in aima-java, who is also working on planning now, and together come to a consensus?

After I checked in, I found a bug that was exposed by one of my test cases, and I realized that the existing Action and PDDL classes were doing some things I should incorporate to fix it. I will make the changes over the next couple days so we're more in sync, and try to come to consensus on the remainder!

@bcorfman please also have a look at #927 and tell me if you want me to change something. My total order planner is a misnomer (and can probably be handled by something as simple as a topological sort on a planning graph) and I'll get rid of it in the next commit but apart from that, the PR heavily relies on the current Action and PDDL classes. It might be a problem if those classes are changed. Do inform me if you really want to though, we can find a way around it. You can also have a look at the planning notebook to see how things are handled now. It's quite different from the time this pull request was first opened. Thanks!

@ad71 I will take a look at #927 and get my bearings ... the basic issue for using any of the search algorithms (A*) for the current KBs is that they must be frozen types (i.e., tuples, frozensets, etc.) to be hashable when stored in the frontier. If we get away from a search representation for the planners (i.e., like you did with Graphplan, correct?), we can stay with the tell() and retract() methods that modify the KB. But otherwise, it seems like we'd be stuck with two different KB representations: frozen and searchable, or mutable and unsearchable. We could move between them, but I don't think we can combine the KB types together in my opinion (at least without an unfortunate breaking of the Python object model, e.g., utils.hashabledict). I always could be missing something though.

@bcorfman we faced similar problems trying to make NQueensProblem and EightPuzzle work with search algorithms, so we decided to let those problems return tuples as states, ie, they didn't use KBs. When we wanted to internally change the state, we converted it to a list, carried out the changes and converted it back. It seems like a lot of work and a lot of duplicated code, but this can be done for KBs as well, though the code would be far from elegant. As of now, I can't think of a better solution...

Sorry I'm so behind -- I fixed the bug I found with my TO planner. I will take a look at #927 over the next couple days and see if we can wrap this up ASAP.

@ad71 I implemented what Dr. @norvig suggested for PlanningAction and PlanningKB ... both of their init methods take strings or Exprs now for a cleaner API.

I agree with you on translating between representations ... your observation is a good reason why Python has both lists and tuples and sets and frozensets, and they can easily be moved between. Do you think we should try the classmethod approach for moving between the types, or use some other way like a standalone function?

Probably my PlanningProblem should be renamed PlanningSearchProblem or something else to distinguish it from the mutable (PDDL) representations. Same for PlanningAction, so it isn't confused with the existing Action class ... I suggest STRIPSAction because of its negative and positive preconditions and add and remove effects that are implemented with set operations. I'm open to improved naming.

As I mentioned previously, strongly think we should rename the PDDL class though (since it has little to do with the PDDL language) ... could we rename that to PlanningProblem instead?

@bcorfman I agree that PDDL is a misleading name. I'll change this in the next update. PlanningSearchProblem and STRIPSAction are good names for your classes. I think you can go ahead with it.
As for translation between representations, I'm fine with anything that works, Dr. @norvig can give you a better suggestion.

Editing notebooks will result in merge conflicts with the other PR. You can add the write-ups later.
If you think you won't have time later on, you can go ahead and add it now, I'll take care of merge conflicts when/if they arise.

No problem; I can wait.

During the interim, I'll work on some unit tests to add to test_planning.py as well.

Thanks @bcorfman. planning.py already provides a total-order solution for planning graphs, so this 2017 PR is superseded; it also includes IDE config files (.idea/) and touches 23 files, which isn't mergeable as-is. Closing — thanks for the contribution!